-
Notifications
You must be signed in to change notification settings - Fork 149
feat: Enhance Course Optimizer Page with Previous Run Links and Improved UI #2356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat: Enhance Course Optimizer Page with Previous Run Links and Improved UI #2356
Conversation
36e5e52
to
33fa7c8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2356 +/- ##
========================================
Coverage 94.50% 94.50%
========================================
Files 1171 1171
Lines 25149 25272 +123
Branches 5374 5568 +194
========================================
+ Hits 23766 23884 +118
+ Misses 1320 1319 -1
- Partials 63 69 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
const PreviousRunLinkHref: FC<{ href: string }> = ({ href }) => { | ||
const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => { | ||
event.preventDefault(); | ||
window.open(href, '_blank'); | ||
}; | ||
|
||
return ( | ||
<div className="broken-link-container"> | ||
<a href={href} onClick={handleClick} className="broken-link" rel="noreferrer"> | ||
{href} | ||
</a> | ||
</div> | ||
); | ||
}; | ||
|
||
const GoToBlock: FC<{ block: { url: string, displayName: string } }> = ({ block }) => { | ||
const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => { | ||
event.preventDefault(); | ||
window.open(block.url, '_blank'); | ||
}; | ||
|
||
return ( | ||
<div className="go-to-block-link-container"> | ||
<a href={block.url} onClick={handleClick} className="broken-link" rel="noreferrer"> | ||
{block.displayName} | ||
</a> | ||
</div> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why adding these functions again,
Can we import these from brokenLinkTable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, refactored the functions accordingly.
<Card className="unit-card rounded-sm pt-2 pb-3 pl-3 pr-4 mb-2.5"> | ||
<p className="unit-header">{unit.displayName}</p> | ||
<DataTable | ||
data={previousRunLinkList} | ||
itemCount={previousRunLinkList.length} | ||
columns={[ | ||
{ | ||
accessor: 'Links', | ||
width: 'col-12', | ||
}, | ||
]} | ||
/> | ||
</Card> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this part is also same, can we add changes to same file BrokenLinkTable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, refactored the same.
const collapsibleTitle = ( | ||
<div className={className}> | ||
<div className="section-collapsible-header-item"> | ||
<Icon src={isOpen ? ArrowDropDown : ArrowRight} /> | ||
<p className="section-title">{title}</p> | ||
</div> | ||
<div className="section-collapsible-header-actions"> | ||
<div className="section-collapsible-header-action-item"> | ||
<p>{previousRunLinksCount > 0 ? previousRunLinksCount : '-'}</p> | ||
</div> | ||
</div> | ||
</div> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use SectionCollapsible?? As only function arguments and collapsibleTitle
is changing in both with same return value including a lot of duplications.
We can edit SectionCollapsible based on a flag i think instead of creating a new component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It is updated as per the suggestion.
const virtualSections = useMemo(() => { | ||
const createVirtualSection = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use some different name? Virtual isn't making any sense, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is updated with meaningful variable name.
// eslint-disable-next-line max-len | ||
() => allSectionsForPrevRun.some(section => section.subsections.some(subsection => subsection.units.some(unit => unit.blocks.some(block => block.previousRunLinks && block.previousRunLinks.length > 0)))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we format this line instead of adding // eslint-disable-next-line max-len
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It's updated.
// eslint-disable-next-line max-len | ||
const hasVisibleUnit = section.subsections.some((subsection) => subsection.units.some((unit) => unit.blocks.some((block) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. can we remove // eslint-disable-next-line max-len
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It's updated.
d88d14c
to
2b44e8f
Compare
subtitle={intl.formatMessage(messages.headingSubtitle)} | ||
/> | ||
<Card> | ||
<div className="d-flex flex-wrap justify-content-between align-items-center mb-3 px-3 py-3"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of adding px-3 py-3
, can we add p-3
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It's updated.
size="md" | ||
className="px-4 rounded-0 scan-course-btn" | ||
onClick={() => dispatch(startLinkCheck(courseId))} | ||
disabled={(!!linkCheckInProgress) && !errorMessage} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use linkCheckInProgress
instead of !!linkCheckInProgress
as it's a boolean field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need !!
because linkCheckInProgress
can be null, but disabled
only accepts true or false. !!
makes sure it’s a valid boolean and also needed for type check.
animation="border" | ||
size="sm" | ||
className="mr-2" | ||
style={{ width: '1rem', height: '1rem' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to scss
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It's updated.
</div> | ||
<Card style={{ boxShadow: 'none', backgroundColor: 'transparent' }}> | ||
<p className="px-3 py-1 small">{intl.formatMessage(messages.description)}</p> | ||
<hr style={{ margin: '0 20px' }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here & everywhere else,
can we move this to scss
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It's updated.
if (linkType === 'previous') { | ||
// Handle previous run links (no filtering, no icons) | ||
if (block.previousRunLinks && block.previousRunLinks.length > 0) { | ||
const blockPreviousRunLinks = block.previousRunLinks.map((link) => ({ | ||
Links: ( | ||
<LinksCol | ||
block={{ url: block.url, displayName: block.displayName || 'Go to block' }} | ||
href={link} | ||
showIcon={false} | ||
/> | ||
), | ||
})); | ||
acc.push(...blockPreviousRunLinks); | ||
} | ||
} else { | ||
// Handle broken links with filtering and icons | ||
if (!filters) { return acc; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conditions in else block are same and in start we have just added early return which neglects/ignores || (!filters.brokenLinks && !filters.externalForbiddenLinks && !filters.lockedLinks)
in below if blocks.
I think we should remove else
and just add if block for previousRunLinks
and keep the other if blocks same. This will also reduce no.of lines git diff, which helps in reviewing faster.
if (linkType === 'previous') {
// Handle previous run links (no filtering, no icons)
if (block.previousRunLinks && block.previousRunLinks.length > 0) {
const blockPreviousRunLinks = block.previousRunLinks.map((link) => ({
Links: (
<LinksCol
block={{ url: block.url, displayName: block.displayName || 'Go to block' }}
href={link}
showIcon={false}
/>
),
}));
acc.push(...blockPreviousRunLinks);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, It's removed
useEffect(() => { | ||
setOpenStates(data?.sections ? data.sections.map(() => false) : []); | ||
}, [data?.sections]); | ||
if (!data?.sections) { | ||
return <InfoCard text={intl.formatMessage(messages.noBrokenLinksCard)} />; | ||
} | ||
setOpenStates(allSectionsForBrokenLinks ? allSectionsForBrokenLinks.map(() => false) : []); | ||
setPrevRunOpenStates(allSectionsForPrevRun ? allSectionsForPrevRun.map(() => false) : []); | ||
}, [allSectionsForBrokenLinks, allSectionsForPrevRun]); | ||
|
||
const { sections } = data; | ||
if (!data) { | ||
return <InfoCard text={intl.formatMessage(messages.noDataCard)} />; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is updated.
e4b049d
to
24befc9
Compare
// Combine renderable sections with regular sections | ||
const allSectionsForBrokenLinks = useMemo( | ||
() => [...renderableSections, ...(sections || [])], | ||
[renderableSections, sections], | ||
); | ||
|
||
const allSectionsForPrevRun = useMemo( | ||
() => [...renderableSections, ...(sections || [])], | ||
[renderableSections, sections], | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we improve this? both seems same to me.
// Calculate previous run links count for each section (including virtual sections) | ||
const previousRunLinksCounts = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual sections?
24befc9
to
1dbf10e
Compare
Description
This PR introduces multiple UI enhancements and new functionality for the Course Optimizer page. The key updates include:
Previous Run Link Section
contentstore.enable_course_optimizer_check_prev_run_links
Course Update, Handouts, and Custom Page Section
UI Enhancements
after-course-optimizer-enhancement-2025-08-08.mp4
Jira
Testing Instructions
Please verify the following: